Block Bindings: Allow editing in post meta source #61753
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SantosGuillamot
added
[Type] Feature
SantosGuillamot
force-pushed
the
try/allow-editing-in-post-meta-bindings
branch
from
2a71c24
to
18e6a5c
Compare
SantosGuillamot
commented
May 17, 2024
|
Size Change: +264 B (+0.02%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
gziolo
reviewed
May 17, 2024
Base automatically changed from
update/block-bindings-lock-with-functions
to
trunk
May 17, 2024 08:47
SantosGuillamot
commented
May 17, 2024
SantosGuillamot
commented
May 17, 2024
SantosGuillamot
force-pushed
the
try/allow-editing-in-post-meta-bindings
branch
from
292b7da
to
fae903b
Compare
|
There is an issue when pressing "Enter" to add a new block, and the rich text is not disabled. I'll take a look at that. |
I think the root of the problem is that when we call Not sure if modifying the attributes in the store itself instead of using the current hook would help in this regard. |
SantosGuillamot
force-pushed
the
try/allow-editing-in-post-meta-bindings
branch
from
995607a
to
b3f4488
Compare
SantosGuillamot
changed the base branch from
trunk
to
try/move-bindings-logic-outside-hook
|
I have rebased this branch into this other pull request because it is supposed to solve the issues I was encountering while pressing Enter on bound blocks. |
29 tasks
gziolo
reviewed
May 24, 2024
| }, | ||
| } ); | ||
| }, | ||
| lockAttributesEditing( { select, context, args } ) { |
There was a problem hiding this comment.
I was a bit confused when I saw this method name as I expected it's an action that locks attribute editing. What alternatives would help here to better express intent – check whether the connected value can be updated?
There was a problem hiding this comment.
You're right it can be confusing. We could change the intent to just the opposite. Some ideas that come to my mind:
allowEditingcanEditSourcecanUpdateSourcecanUpdateSourceValue
Any ideas are welcome 🙂
There was a problem hiding this comment.
One important consideration is that the user might be able to edit source, but not necessarily all posts or individual fields, as permissions are very granular. I don't know if that gets reflected on the client.
I see that permissions are checked only on the post level with canUserEditEntityRecord . In effect, canUserEditValue or canEditValue might resonate better.
There was a problem hiding this comment.
I've changed the name to canUserEditValue in the latest commit. It looks better to me. Anyways, the API is still private so we can change that if wanted.
One important consideration is that the user might be able to edit source, but not necessarily all posts or individual fields, as permissions are very granular. I don't know if that gets reflected on the client.
Do you mean that the current conditions might not be enough? We are assuming that if these conditions are true, the user can edit the meta field:
- The user can edit the current post.
- The meta field is publicly exposed in the REST API.
What else should we consider?
There was a problem hiding this comment.
I was thinking about the is_protected_meta function that allows filtering, but you are correct that when the individual meta fields gets exposed through REST API for the current user (and their role) then we can safely assume they can edit it 👍🏻
SantosGuillamot
force-pushed
the
try/move-bindings-logic-outside-hook
branch
from
ed11e1a
to
3165915
Compare
SantosGuillamot
force-pushed
the
try/allow-editing-in-post-meta-bindings
branch
2 times, most recently
from
5fdbacb
to
8e6dd67
Compare
|
Flaky tests detected in 570a4b2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9251566900
|
SantosGuillamot
force-pushed
the
try/move-bindings-logic-outside-hook
branch
from
c8fd783
to
629056c
Compare
SantosGuillamot
force-pushed
the
try/allow-editing-in-post-meta-bindings
branch
from
570a4b2
to
d35db11
Compare
SantosGuillamot
changed the base branch from
try/move-bindings-logic-outside-hook
to
trunk
SantosGuillamot
force-pushed
the
try/allow-editing-in-post-meta-bindings
branch
from
9bfc757
to
7f8771e
Compare
SantosGuillamot
changed the base branch from
trunk
to
update/create-utils-for-block-bindings
SantosGuillamot
force-pushed
the
try/allow-editing-in-post-meta-bindings
branch
from
2012ae8
to
c72afda
Compare
Did some testing, I'm not seeing any issues 👍 |
| registry.dispatch( noticesStore ); | ||
| createWarningNotice( | ||
| __( | ||
| "Blocks can't be inserted into other blocks with bindings" |
There was a problem hiding this comment.
You can paste a paragraph inside a binded paragraph to update its content right?
I could do that on my testing, so in that case, we may change the warning to
Blocks can't be insterted into different blocks with bindings
There was a problem hiding this comment.
You can paste a paragraph inside a binded paragraph to update its content right?
Mmm, I don't think so. You can paste some text, but not if you copy the entire block I believe.
There was a problem hiding this comment.
In my testing, it largely depend when you try to paste blocks. For example, try it with the Heading block, the Verse block, or caption for the Image block. It will get handled differently without any notices in the UI:
Screen.Recording.2024-05-31.at.12.54.22.mov
In effect, my recommendation would be to take some action that fits best in the given scenario and skip the warning.
There was a problem hiding this comment.
Ideally, we probably want to modify the source value with whatever is supposed to be pasted. But that seemed a bit tricky to me at this point and I thought it could be handled in a follow-up PR. That's why I added the warning in the meantime.
| ); | ||
| return; | ||
| } | ||
| dispatch.insertAfterBlock( selectionA.clientId ); |
There was a problem hiding this comment.
Suggested change
| dispatch.insertAfterBlock( selectionA.clientId ); |
I would remove the enter option, is something we can add in future releases after some UX research.
cbravobernal
previously approved these changes
May 30, 2024
cbravobernal
dismissed
their stale review
I found that if you edit the custom field meta box, the editing stops from working.
cbravobernal
approved these changes
May 30, 2024
ellatrix
reviewed
May 31, 2024
| }, | ||
| } ); | ||
| }, | ||
| canUserEditValue( { select, context, args } ) { |
There was a problem hiding this comment.
Yes, registering block binding sources is still private. If everything goes well in WP 6.6 cycle, we are going to open it to everyone in WP 6.7.
There was a problem hiding this comment.
Yes, and we will use next cycle to polish the APIs. There are some changes I would like to explore.
|
It's rather unrelated to this PR, but I wanted to surface it as I noticed while testing: 
I tried to set a binding on the |
|
Everything works correctly in my testing. It looks like permissions are respected, so when editing the post as an author, when I use Query block to display other posts, I can't modify meta values for fields I don't have access to: Screen.Recording.2024-05-31.at.13.17.36.movOverall, I can see only two aspects to watch after this very exciting feature gets enabled for users:
Screen.Recording.2024-05-31.at.13.26.20.mov |
gziolo
approved these changes
May 31, 2024
There was a problem hiding this comment.
Excellent work bringing it to the finish line. This is extremely exciting to see how easy it becomes to edit post meta inline 🎉
I did a very extensive testing using the admin and author roles, all supported block types: Paragraph, Heading, Button and Image, product and site editor. I couldn't discover any blockers. I would be in favor of including it in WordPress 6.6 release.
|
Thanks a lot for the extensive testing, Greg! Much appreciated I totally agree that we will need to keep iterating on the "Enter" and "Paste" workflows, so let's keep an eye on that. Also, once it is out we will get community feedback that can help make decisions.
Regarding this, I just created this pull request to remove them from the bindings panel. |
bph
added
[Type] Enhancement
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jun 4, 2024
* Create bindings util for transforming block attributes * Get attributes and name from the store * Change function to return only the bound attributes * Add action, selector, and reducer for block context * Sync store in edit component * Revert "Sync store in edit component" This reverts commit 988c4b6. * Move logic to BlockContextProvider * Change parent context logic * Use useLayoutEffect * Go back to syncing store in edit component * WIP: Move bindings logic to `getBlockAttributes` * WIP: Move bindings setAttributes logic to updateBlockBindings * Pass only `select` to `getValue` functions * Remove old editor hook * Add fallback to postId until context is ready * Remove setValue post-meta code * Simplify fallback conditional * Change bindings destructuring * Check canBindAttribute in updateBlockAttributes * Update unit tests to expect a dispatch * Add conditional in block * Don't use `getBlockAttributes` inside `getValue` * Revert "Don't use `getBlockAttributes` inside `getValue`" This reverts commit 0e91129. * Avoid processing bindings recursively * Access context through selector * Update getBlockAttributes logic * Don't use fallbacks * Add edit value posibility for post meta, add function to check if is admin * Revert "Add edit value posibility for post meta, add function to check if is admin" This reverts commit 9659455. * Test editing is allowed in paragraph block * Test protected fields are not editable * Revert "Enable parallel processing for PHPCS sniffs (WordPress#61700)" This reverts commit 8331820. * Add post meta setValue function * Update tests to check contenteditable * Update lockAttributesEditing default * Pass arguments to lockAttributesEditing * Check user can edit post meta * Check field is exposed in the REST API * Disable editing in templates * Add fallback for postId * Revert "Revert "Enable parallel processing for PHPCS sniffs (WordPress#61700)"" This reverts commit e74d71c. * Adapt old tests * Simplify lockAttributesEditing fallbacks * Don't use fallback for context * Add postType fallback when locking controls * Pass context in rich text * Check contenteditable attribute in test * Change name to `canUserEditValue` * Revert changes caused by rebasing * Add space back * Pass block context through rich text * Change imports * Transform block attributes into bindings in split selection * Pass context to in use-input * Change REST API check * Use getBoundAttributesValues * Don't split when attribute is bound * Revert changes caused by rebase * Cover more blocks when editing custom fields * Add a warning when pasting blocks --------- Co-authored-by: Carlos Bravo <carlos.bravo@automattic.com>
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
* Create bindings util for transforming block attributes * Get attributes and name from the store * Change function to return only the bound attributes * Add action, selector, and reducer for block context * Sync store in edit component * Revert "Sync store in edit component" This reverts commit 988c4b6. * Move logic to BlockContextProvider * Change parent context logic * Use useLayoutEffect * Go back to syncing store in edit component * WIP: Move bindings logic to `getBlockAttributes` * WIP: Move bindings setAttributes logic to updateBlockBindings * Pass only `select` to `getValue` functions * Remove old editor hook * Add fallback to postId until context is ready * Remove setValue post-meta code * Simplify fallback conditional * Change bindings destructuring * Check canBindAttribute in updateBlockAttributes * Update unit tests to expect a dispatch * Add conditional in block * Don't use `getBlockAttributes` inside `getValue` * Revert "Don't use `getBlockAttributes` inside `getValue`" This reverts commit 0e91129. * Avoid processing bindings recursively * Access context through selector * Update getBlockAttributes logic * Don't use fallbacks * Add edit value posibility for post meta, add function to check if is admin * Revert "Add edit value posibility for post meta, add function to check if is admin" This reverts commit 9659455. * Test editing is allowed in paragraph block * Test protected fields are not editable * Revert "Enable parallel processing for PHPCS sniffs (WordPress#61700)" This reverts commit 8331820. * Add post meta setValue function * Update tests to check contenteditable * Update lockAttributesEditing default * Pass arguments to lockAttributesEditing * Check user can edit post meta * Check field is exposed in the REST API * Disable editing in templates * Add fallback for postId * Revert "Revert "Enable parallel processing for PHPCS sniffs (WordPress#61700)"" This reverts commit e74d71c. * Adapt old tests * Simplify lockAttributesEditing fallbacks * Don't use fallback for context * Add postType fallback when locking controls * Pass context in rich text * Check contenteditable attribute in test * Change name to `canUserEditValue` * Revert changes caused by rebasing * Add space back * Pass block context through rich text * Change imports * Transform block attributes into bindings in split selection * Pass context to in use-input * Change REST API check * Use getBoundAttributesValues * Don't split when attribute is bound * Revert changes caused by rebase * Cover more blocks when editing custom fields * Add a warning when pasting blocks --------- Co-authored-by: Carlos Bravo <carlos.bravo@automattic.com>
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Use the latest changes to the block bindings API in the editor to allow users to edit the value of the custom fields directly through the blocks when they are connected. For example, when the paragraph content is bound to a specific custom field, and the user start typing, they would be modifying the custom field and not the content.
At least in this first iteration, it should be limited to:
How?
Using the
setValuefunction based on the code created by @ellatrix here. Apart from that, we should use thelockAttributesEditingcallback added in this pull request to cover the mentioned limitation.Additionally, it ensure that the split selection work as expected in bound blocks.
Testing Instructions
I added a couple of e2e tests for this use case. You can test it manually by:
Testing Instructions
1. Register post meta by adding this snippet to your theme's functions.php
2. Add a paragraph block bound to the custom field using the Code Editor